-
Notifications
You must be signed in to change notification settings - Fork 19
Draft: Proposed clocks improvements #71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
c8c3316
to
205bdc8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this change overall. I've added some feedback on the actual changes plus some other notes that can be punted to a separate PR if you don't want to deal with them.
205bdc8
to
c91853b
Compare
I've rebased this on top of #79. The changes added in this PR can be seen in bakkot/wasi-clocks@clocks-improvements-on-three...ptomato:wasi-clocks:proposed-clocks-improvements. |
Co-authored-by: Philip Chimento <[email protected]>
Co-authored-by: Philip Chimento <[email protected]>
@ptomato, sorry, I rebased my PR and broke this one again. Working link to view changes in this PR: bakkot/wasi-clocks@f1a63db...ptomato:wasi-clocks:proposed-clocks-improvements |
The "timezone of an instant" is a misnomer because the `instant` type doesn't carry timezone information. The host has a currently configured time zone (or has no such concept, in which case the only sensible thing to do is use UTC).
This flag is problematic because when a time zone is "in DST" is not well defined. Most time zones in the world don't use DST at all. Of the ones that do, most go to DST during the summer for half the year, but not all. For example, the function in Moment.js that provides this functionality comes with a giant caveat: https://momentjs.com/docs/#/query/is-daylight-saving-time/
Other possibilities: "tzid", "iana-id", "identifier", "iana-identifier". Returning a user-displayable name as part of timezone-display would require more information: the user's preferred language, and the preferred style for the name: - abbreviated or not - year-round or specific to the Instant E.g., the time zone with the IANA id "America/Los_Angeles" might be displayed as "Pacific Time", "Pacific Standard Time", "Pacific Daylight Time", "PT", "PST", "PDT", "Nordamerikanische Westküstenzeit"... The Rust iana_time_zone crate uses IANA time zone IDs, so if this interface needs to be able to implement iana_time_zone, timezone-display should have an IANA ID and not a user-displayable name.
There are time zones that used sub-minute or even sub-second UTC offsets for instants in the past. E.g., when built using Vanguard format, the UTC offset in the TZDB for "Asia/Ho_Chi_Minh" before July 1906 is 7:06:30.133333333.
If the timezone-display ID is an IANA ID, and we are going with the approach of not making the localized ("PST" vs "PDT" vs "PT") name part of this component, then the current time zone doesn't depend on the current time. After removing the isDST flag, timezone-display contains two pieces of data, the ID and the UTC offset. The UTC offset is already available via a function that takes an Instant as input. The ID could just be available via its own function that doesn't take any input. In that case there would be no need for timezone-display.
Instead of only specifying the epoch of the instant returned from now(), specify the epoch of the instant type. It then follows that now() returns an instant with that epoch.
c91853b
to
0c3e042
Compare
OK, I pulled in Kevin's branches, this should be the comparison link now: ptomato/wasi-clocks@b340dbe...ptomato:wasi-clocks:proposed-clocks-improvements |
We wish to be able to represent "is completely unaware of time zones" or "failure to discover a time zone" in the API, distinct from a host that always uses UTC.
Use types::duration for the return type of get-resolution instead. It supports durations up to 584 years which is more than sufficient for any clock resolution. (A more capable duration type would be needed if we were doing any arithmetic between instants.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small suggestion but overall looking good. Thanks for this!
|
||
/// The same as `display`, but only return the UTC offset. | ||
/// The number of nanoseconds difference between UTC time and the local |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be moderately inclined to leave this as seconds. There is some appeal to being consistent with duration
but I think it makes more sense to match instant.seconds
here.
From what I've been reading there shouldn't be any need for sub-second offsets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd encourage sticking with nanoseconds here — we found a number of cases in the tzdata where there were sub-second offsets. See Justin's original recommendation at #61 (comment)
If the concern is that it would be easier to just do instant.seconds - offset
or whatever to get the local wall-clock time, then I'd suggest instead adding a method that does that, because you probably want wall-clock time to be expressed in Y-M-D-H-M-S and not epoch nanoseconds...
See #69 (comment). This is intended as a starting point for discussion about what's possible.